Add generic policy handler; refactor IAMAuthPolicy, TargetGroupPolicy, VpcAssociationPolicy #547

mikhail-aws · 2023-11-28T05:40:30Z

Note

Add generic policy handler and refactor IAMAuthPolicy, TargetGroupPolicy, VpcAssociationPolicy with use of generic handler.

Testing:

e2e-test
Manual tests around setting correct statuses for different policies

Generic Policy Handler

The high-level design is to introduce generic helper struct with collection of different methods to work with Policy CRD's. This helper/handler is supposed to be used in controllers that interact with Policy CRDs.

Controller's code should explicitly call different, and relatively small methods of handler, so that it should be easy to tell what controller does looking at controller's code.

Code includes some arcane golang around generics. For example:

func NewPolicyHandler[T, TL any, P policyPtr[T], PL policyListPtr[TL, P]](cfg PolicyHandlerConfig) *PolicyHandler[P] {

These 4 generic types methods are results of having 2 types of policy structs Policy and PolicyList in every CRD. Also I need to make distinction between Policy and ref type *Policy for some functionality related to k8s client calls - Get and List.

All generic complexity stays in Policy file but interface is still simple with single type and I believe understandable methods.
PolicyHandler creation is as simple as it can be.

         type iap = anv1alpha1.IAMAuthPolicy
         type iapl = anv1alpha1.IAMAuthPolicyList

         phcfg PolicyHandlerConfig := PolicyHandlerConfig{}
         _ = NewPolicyHandler[iap, iapl](cfg: phcfg)

It's a not valid config to call all methods, but it's enough for compiler to understand it's IAMAuthPolicy and all related methods will be statically checked.

Partly address: #508

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

pkg/k8s/policyhelper/policy.go

pkg/controllers/iamauthpolicy_controller.go

pkg/k8s/policyhelper/policy.go

pkg/k8s/policyhelper/kind.go

pkg/controllers/iamauthpolicy_controller.go

solmonk · 2023-11-29T21:47:18Z

I was thinking of getting rid of pointer definition and came up with this example, what do you think?
https://github.com/solmonk/aws-application-networking-k8s/blob/refactor-policy-poc/pkg/k8s/policyhelper/policy.go#L229-L237
You might not like using runtime reflect functions but we can back this up by having proper unit test case. (https://github.com/solmonk/aws-application-networking-k8s/blob/refactor-policy-poc/pkg/k8s/policyhelper/policy_test.go#L247-L256)

pkg/k8s/policyhelper/kind.go

pkg/k8s/policyhelper/policy.go

mikhail-aws · 2023-11-29T22:04:22Z

I was thinking of getting rid of pointer definition and came up with this example, what do you think? https://github.com/solmonk/aws-application-networking-k8s/blob/refactor-policy-poc/pkg/k8s/policyhelper/policy.go#L229-L237 You might not like using runtime reflect functions but we can back this up by having proper unit test case. (https://github.com/solmonk/aws-application-networking-k8s/blob/refactor-policy-poc/pkg/k8s/policyhelper/policy_test.go#L247-L256)

4 tuple of types isnt that bad as it looks like, especially pointerType can be derived from structType and I dont need explicit type declaration. I dont think it worth replacing it with reflection. I end up with smaller helper constructor functions that simplifies usage outside of policy.go.

As I mentioned before we still need to maintain mapping between single and plural crd structs.

For example:

 func NewVpcAssociationPolicyHandler(log gwlog.Logger, c k8sclient.Client) *PolicyHandler[*VAP] {
         phcfg PolicyHandlerConfig := PolicyHandlerConfig{
                 Log:            log,
                 Client:         c,
                 TargetRefKinds: NewGroupKindSet(objs...: &gwv1beta1.Gateway{}),
         }
         return NewPolicyHandler[VAP, VAPL](cfg: phcfg)
 }

pkg/k8s/policyhelper/policy.go

xWink

Some small comments. Also would like to see unit tests on the new policy helper functions.

solmonk · 2023-11-29T22:30:49Z

I feel like this PR kinda gives a vibe of a sophisticated TypeScript code doing type circuses.

Doing a recap of why I thought this was hard to read:

need to understand relationship between T, U, P, and PL. so P is a pointer of T and PL is a pointer of U
need to understand relationship between type constraints in different structs. policyListPtr's T is policyClient's TL
the name policyPtr sounds like a interface ptr but actually a struct ptr pointing to underlying policy implementation
U doesn't relate to any initial of an object, should be TL or something else?
T is an any type which doesn't say much by itself, and can't actually constraint anything (I think this is most critical)

If we manage to reduce it to just P and PL I think it is more consistent and readable (I mean, except for a few lines of reflection magic)

And I'm okay with having both single and list types on handler generics. I think it has benefits, as you mentioned.

mikhail-aws · 2023-11-29T23:20:20Z

@solmonk I agree it would be nicer to have only 2 types: policy and list. I dont mind to follow up after this pr, currently this behavior is isolated to local k8sClient struct and not exposed outside, it should be an easy change after.

mikhail-aws · 2023-11-30T02:15:05Z

@xWink addressed minor comments, but not unit tests. I will do follow-up on this PR with unit tests, currently it's being manually tested and ran full e2e test suite.

xWink · 2023-11-30T04:42:37Z

LGTM. Thanks for putting this together!

mikhail-aws added 4 commits November 23, 2023 16:15

add v1 to controller's scheme

308bc7e

add generic policy handler and use it in iam auth policy

e68deab

minor update

94a5917

policy handler - move watchers from struct field into func args

af87744

solmonk changed the title ~~Add generic policy handler and refactor IAM Auth Polciy~~ Add generic policy handler and refactor IAM Auth Policy Nov 28, 2023